Skip to content

Conversation

@Ace-Tang
Copy link
Contributor

ci test alway hang on installing criu things some times ago, add
log to make sure who blocked the ci, see #2298.

Signed-off-by: Ace-Tang [email protected]

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

no need.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Oct 18, 2018

Codecov Report

Merging #2323 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
+ Coverage   67.34%   67.37%   +0.02%     
==========================================
  Files         218      218              
  Lines       17674    17674              
==========================================
+ Hits        11902    11907       +5     
  Misses       4365     4365              
+ Partials     1407     1402       -5
Flag Coverage Δ
#criv1alpha1test 32.33% <ø> (+0.02%) ⬆️
#criv1alpha2test 36.21% <ø> (-0.04%) ⬇️
#integrationtest 40.44% <ø> (-0.02%) ⬇️
#nodee2etest 33.59% <ø> (+0.02%) ⬆️
#unittest 23.32% <ø> (ø) ⬆️
Impacted Files Coverage Δ
ctrd/image.go 76.75% <0%> (-2.2%) ⬇️
cri/v1alpha2/cri.go 67.88% <0%> (-0.25%) ⬇️
cri/v1alpha2/cri_wrapper.go 61.2% <0%> (ø) ⬆️
daemon/mgr/container.go 59% <0%> (+0.21%) ⬆️
cri/v1alpha1/cri.go 61.03% <0%> (+0.32%) ⬆️
ctrd/container.go 59.28% <0%> (+0.95%) ⬆️
daemon/containerio/container_io.go 77.04% <0%> (+1.09%) ⬆️
daemon/containerio/cri_log_file.go 88.23% <0%> (+3.92%) ⬆️

@fuweid
Copy link
Contributor

fuweid commented Oct 18, 2018

interesting. the change is LGTM but I didn't see any output in the CI log. 🤔

hack/install/install_criu.sh
>>>> install criu <<<<
Extracting templates from packages: 100%
Cloning into '/tmp/criu-build-rqlfbo/criu'...
remote: Enumerating objects: 80, done.
remote: Counting objects: 100% (80/80), done.
remote: Compressing objects: 100% (46/46), done.
remote: Total 60512 (delta 41), reused 58 (delta 34), pack-reused 60432
Receiving objects: 100% (60512/60512), 15.80 MiB | 22.79 MiB/s, done.
Resolving deltas: 100% (44178/44178), done.
Switched to a new branch 'v3.10'

@fuweid
Copy link
Contributor

fuweid commented Oct 18, 2018

my bad. Please remove the redirection.

criu::ubuntu::install_dependencies > /dev/null

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the redirection or think other way to show the comment

ci test alway hang on installing criu things some times ago, add
log to make sure who blocked the ci, see #2298.

Signed-off-by: Ace-Tang <[email protected]>
@Ace-Tang
Copy link
Contributor Author

Keep redirection, echo log before function, or log will too many, I did not consider this before.

os_dist="$(detect_os)"
if [[ "${os_dist}" = "Ubuntu" ]]; then
criu::ubuntu::install_dependencies > /dev/null
echo ">>>> start to download criu from github repository <<<<"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the downloading task fails, would this script show the detailed reason why it fails? @Ace-Tang

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only know we stack at downing from github.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then it meets my demand.

echo ">>>> install criu <<<<"

os_dist="$(detect_os)"
if [[ "${os_dist}" = "Ubuntu" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add one comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think so, >>>> install criu <<<< will tell begin to download dependencies. and >>>> start to download criu from github repository <<<< will tell begin to download from github. Then we can know which step stack.

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants